feat: Improve error handling of internal sentry client #9826
Conversation
dcramer
commented
Sep 19, 2018
dcramer
commented
- Handle IntegrityError when fetching credentials
- Switch to machine-readable log events
- Mimic correct endpoint with request path
- Correct invalid test (passing project vs project_id to EventManager.save)
| environment='totally unique super duper environment', | ||
| )) | ||
| event = manager.save(project) | ||
| event = manager.save(project.id) |
There was a problem hiding this comment.
noidea.jpg, but it was failing for me
There was a problem hiding this comment.
As far as I can tell the call to Project.objects.get_from_cache used to be a cache hit and now it isn't anymore. In case of a cache miss it can't deal with passing a Model, in case of a cache hit it can.
We could just remove the logic that converts Model instances to their PKs in the model manager's get_from_cache since nobody can rely on it anyway. Opinions?
|
The goal of this PR is to stop all of the other tickets where, for example, the migration fails to create sentry_project (correctly), and people keep opening github issues pointing to a wrong root cause. |
| if key is None: | ||
| return | ||
|
|
||
| if not is_current_event_safe(): |
There was a problem hiding this comment.
interestingly we call this in both capture() and send(), and I'm not really sure if we need both (but I also dont have the time or care to investigate if we can remove one)
|
|
||
| key = None | ||
| try: | ||
| if settings.SENTRY_PROJECT_KEY is not None: |
There was a problem hiding this comment.
Do we use this setting in production? I couldn't find it in the repository.
There was a problem hiding this comment.
Don’t think so. I forget why it’s a thing.
There was a problem hiding this comment.
It appears to have shown up in #6785 if we aren't using it do we need to keep it?
There was a problem hiding this comment.
Only real concern would be is if anyone uses it in OS (assuming it still has valid use).
There was a problem hiding this comment.
Probably best to leave it alone then.
|
Test failures here are super cryptic |
dcramer
force-pushed
the
feat/improve-internal-client-reporting
branch
2 times, most recently
from
a19116a to
5dadb70
Compare
- Handle IntegrityError when fetching credentials - Switch to machine-readable log events - Mimic correct endpoint with request path - Correct invalid test (passing project vs project_id to EventManager.save)
dcramer
force-pushed
the
feat/improve-internal-client-reporting
branch
from
5dadb70 to
31e1f88
Compare
|
That was painful to debug. Importing 'sentry.utils.cache' in this module caused the cache to semi-configure in a broken way. It created a lot of cryptic errors (mostly event processing was broken, due to the event never getting stored into the cache). Swapped it out with Django's cached_property instead. |
